Can we initialize mss_now using tcp_current_mss() and get rid of mss_now != 0?

I think about it, but we don't need to calculate mss_now if SOCK_TEMPESTA_HAS_DATA is not set and tcp_send_head is empty. I don't want to make extra calculations

(1) Why do we recalculate limit here? We have limit that calculated in tcp_write_xmit, the limit calculated correctly and we can use it.

Because we want to make one TLS record for several skbs. For example if mtu is equal 1500, our previously calculated limit is also about 1500, but if there are several skbs in our socket write queue we can encrypt them all in only one tls record! And do not call tls_encrypt several times

(2) This is looks not optimal. We calculate a limit, then in TFW_ADJUST_TLS_OVERHEAD we reduce the limit by tls overhead, the limit becomes lower than skb->len thus tso_fragment will split this skb, this is unnecessary overhead. After in tfw_tls_encrypt we will encrypt boths skbs, but the last one anyway will hang in write_queue.

Yes because we encrypt several skbs we don't need to adjust overhead

Still need flags to notify when skb is transmitted, e.g. tempesta-tech/tempesta#2108.
Of course, on_tcp_entail can do this as well, but it is not currently used for this purpose, at least not for all control frames.

We should implement additional on_send_ function in tempesta for 2108, not use flags in skb. Moreover we remove additional callback from kernel code, so 2108 should be reworked

No, it can't be in on_send callback, it only indicates entry into the write queue of sk, but the counter decrement must happen at the time of actual transmission, because only at that point in time the skb is moved to the TCP protocol transmission process, i.e. tcp_write_xmit or its callers, e.g. __tcp_push_pending_frames, and not accumulated in kernel memory for some reason, such as congestion or zero TCP window of the malicious peer. This is exactly the meaning of this counter, to avoid control frame flooding.

That is, for 2108, the solution is either to keep the counter decrement in sk_write_xmit, and the tfw_cb.flag is still needed, or to manipulate the counter directly in ss_skb_on_tcp_entail. No additional callbacks needed here.

ss_skb_on_tcp_entail is called before pushing skb to socket write queue. It is a good place to decrement counter.

Applications which works in user space and counted control frames don't know nothing about __tcp_push_pending_frames and there is no problem.

I think we can postpone such frames if it is really necessary (and send them only when we have enough window) and count them in ss_skb_on_tcp_entail. I think that we move tls_encrypt to Tempesta code and there is no sense to have one more callback in linux kernel only to count control frames. User space applications like nginx doesn't know anything about tcp_transmit_skb. How they can count control frames?

User space applications like nginx doesn't know anything about tcp_transmit_skb. How they can count control frames?

@EvgeniiMekhanik @krizhanovsky @const-t

I have to update some thoughts.

No, the user program relies on sockbuf to determine whether the skb send is in-flight. If sockbuf is full, the send() system call will fail because the peer does not provide enough window, or in non-blocking fd, the write event is shelved, then the counter will accumulate, and then the user program will know that it is in a flood attack.

(Moreover, the user program can use TCP_INFO getsockopt to retrieve the TCP information to get known the in-flight bytes. In fact, Nginx uses it to define some variables, although not for our purpose exactly.)

However, in our case, it's a completely different story. tempesta does not use sockbuf, but directly enqueues to sk's sk_write_queue, so it will have memory risks unless we know that skb has started to be transmitted, that is, in-flight. How to know this fact? The only place is that tcp_transmit_skb successfully handles skb. So this is why I suggest that we should apply the counting logic there. Neither on_send (called in ss_tx_action) nor entail (corresponding to the original prepare_xmit), or even write_xmit callback, can meet our needs to mitigate the flood of control frame attacks. This is not a theory, but has been verified through testing.

And the method suggested by @EvgeniiMekhanik does not work either, because similar to entail, the skbs pushed when the window is available does not mean that all skbs can be sent immediately, because it does not know how large the network capacity is and how many skbs can be sent, so it's likely that some skbs will be hold in the memory for an undetermined time.

We call ss_skb_on_tcp_entail just before we push skb to socket write queue. For HEADERS and DATA frames we calculate tcp window in sk_fill_write_queue and don't call ss_skb_tcp_entail->ss_skb_on_tcp_entail if we have no enough window. To solve problem with control frames we should not call ss_skb_tcp_entail->ss_skb_on_tcp_entail for all other type of frames if there is no enough tcp window. We should push them to the queue of postponed frames and push them to socket write queue only when we have enough TCP window. In this case we can counted control frames in ss_skb_on_tcp_entail

Fair enough. This is sufficient if we do not care about the strict accuracy of whether the skb was successfully sent by tcp_transimit_skb or was even eliminated from tcp_rtx_queue.

I agree that the flags are replaced with the on_send callback in #1973, so it's not desired to keep flags, especially with only one user. Meantine, I also don't like the idea of additional queueing since to wasted memory decreasing performance and opens opportunities for DoS attacks.

However, in first place, I didn't get what do we fix with #2108, so I requested changes. Maybe I'm wrong and let's discuss the fix, but I proposed to control and limit flows to client in tempesta-tech/tempesta#498 (comment)

P.S. @kingluo @EvgeniiMekhanik thank you for inviting me to the discussion

I think we should document this breaking change, explaining why sk_prepare_xmit is deprecated, and why the new sk_fill_write_queue requires some somewhat complicated limit calculations. If not, maybe the author himself will forget about it after a few weeks. It's better to document it in the code as a comment, because tempesta-tech/tempesta#1196 only discusses some theoretical design (which is even outdated after the code rewrite), not the actual implementation.

Yeah, it could make sense to document both the two callback, when they are called and what they do

Personally, I don't quite understand why we moved frames from prepare_xmit to tcp_push_pending_frames, which is the biggest change in this PR. In prepare_xmit, we already know that TCP has enough window to send data, but in tcp_push_pending_frames, we have to calculate it ourselves, which seems to be duplication of work. Could you clarify this further in comments?

Yes, I will add comment before merge PR. There were several problems with the previous approach:

1. We push response skb directly to socket write queue. If skb belongs to stream, which is blocked by exceeding HTTP2 stream window, we can't send this skb to the client. This skb blockes all other skb in socket write queue, until window update will come.
2. We can't prioritize streams. We push responses to socket write queue directly, so there is no prioritization. Now we push response skbs to our private scheduler queue. Then we call tcp_push_pending_frames where we run our scheduler to choose the most priority stream and send data according TCP window. Also tcp_push_pending_frames called from tcp_data_snd_check when we receive data from the peer and TCP window opened again.